Use representation instances for reducers #1089
Conversation
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @jamesbradleym)
Elements.MEP/src/Fittings/Reducer.cs line 75 at r1 (raw file):
.Concat(this.End.GetArrow(endNodeTransform.Origin)).Concat(GetExtensions()); this.RepresentationInstances.Add(new RepresentationInstance(new SolidRepresentation(sweep1), this.Material));
This is not really the pattern we want when we use representation instances. The goal of representation instances was to make it so we re-use representation, but with how you have done it here we're making new instances every time. See the UpdateRepresentation method in Elbow.cs
However, if I recall correctly getting reducer representations to appear correctly was tricky because they both have a direction, large->small vs small->large, and they can be eccentric, which means their transform orientation is that much trickier.
Do we have a theory about why this use of reprsentation instances works when old style representations doesn't work?
There was a problem hiding this comment.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)
Elements.MEP/src/Fittings/Reducer.cs line 75 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
This is not really the pattern we want when we use representation instances. The goal of representation instances was to make it so we re-use representation, but with how you have done it here we're making new instances every time. See the
UpdateRepresentationmethod in Elbow.csHowever, if I recall correctly getting reducer representations to appear correctly was tricky because they both have a direction, large->small vs small->large, and they can be eccentric, which means their transform orientation is that much trickier.
Do we have a theory about why this use of reprsentation instances works when old style representations doesn't work?
Using FittingRepresentationStorage.SetFittingRepresentation results in bad geometry transforms it looks like, is that the right implementation? I had taken this pattern from https://github.com/hypar-io/AECTech2023/blob/0e6db42d4f20bc85a03d565cdd16a7a1e726a063/ExampleFunctions/Precincts/dependencies/Site.cs#L15-L22 which I thought was on the newer side but maybe that wasn't the right place to look.
There was a problem hiding this comment.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @jamesbradleym)
Elements.MEP/src/Fittings/Reducer.cs line 75 at r1 (raw file):
Previously, jamesbradleym wrote…
Using
FittingRepresentationStorage.SetFittingRepresentationresults in bad geometry transforms it looks like, is that the right implementation? I had taken this pattern from https://github.com/hypar-io/AECTech2023/blob/0e6db42d4f20bc85a03d565cdd16a7a1e726a063/ExampleFunctions/Precincts/dependencies/Site.cs#L15-L22 which I thought was on the newer side but maybe that wasn't the right place to look.
hmmm... yea, that rings a bell, other fittings can more easily have a transform that orients them in space, but we never made transforms of reducers make total sense...
ok, it's not important to solve now, let's just add a TODO here that explains that we'd rather use representationInstances with a Factory pattern like other fittings because we want the advantage of instancing geometry, but because reducers have unique orientation requirements we haven't implemented it yet.
There was a problem hiding this comment.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)
Elements.MEP/src/Fittings/Reducer.cs line 75 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
hmmm... yea, that rings a bell, other fittings can more easily have a transform that orients them in space, but we never made transforms of reducers make total sense...
ok, it's not important to solve now, let's just add a TODO here that explains that we'd rather use representationInstances with a Factory pattern like other fittings because we want the advantage of instancing geometry, but because reducers have unique orientation requirements we haven't implemented it yet.
In that case could we just modify the SetFittingRepresentation to work with Reducer as well? As implemented: Add a boolean for transforming and a boolean for unioning?
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @jamesbradleym)
Elements.MEP/src/Fittings/Reducer.cs line 75 at r1 (raw file):
Previously, jamesbradleym wrote…
In that case could we just modify the
SetFittingRepresentationto work withReduceras well? As implemented: Add a boolean for transforming and a boolean for unioning?
oh sorry, since I don't see a representation hash method being implemented, which means we're not actually re-using the representations from the storage, I think we should just leave the TODO. as it is now we're sort of looking like we're using the storage but we're not really, representations won't ever be re-used, unless i'm mis-reading something.
There was a problem hiding this comment.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)
Elements.MEP/src/Fittings/Reducer.cs line 75 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
oh sorry, since I don't see a representation hash method being implemented, which means we're not actually re-using the representations from the storage, I think we should just leave the TODO. as it is now we're sort of looking like we're using the storage but we're not really, representations won't ever be re-used, unless i'm mis-reading something.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status:complete! 1 of 1 approvals obtained
BACKGROUND:
DESCRIPTION:
this.Representationtothis.RepresentationInstancesfor Reducer representations.TESTING:
FUTURE WORK:
REQUIRED:
CHANGELOG.md.COMMENTS:
This change is